-
Notifications
You must be signed in to change notification settings - Fork 71
Valkey helm #419
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Valkey helm #419
Conversation
Signed-off-by: cherukum-amazon <[email protected]>
Signed-off-by: Silvio Gissi <[email protected]>
|
@cherukum-Amazon @stockholmux Can you take a look at this update please? It has a dependency on a new release of valkey-helm supporting replicas (valkey-io/valkey-helm#84) which I'm merging shortly. |
| OK | ||
| $ $NEW_VALKEY_CLI info | grep '^\(role\|master_host\|master_link_status\)' | ||
| role:slave | ||
| master_host:valkey-bitnami-primary |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this the instance name, can we map to an existing environment variable?.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also keep environment variables in one place with comments, so its easy to back track.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The shell variable here is misleading. I'll update to create a shell alias instead.
| Update all clients to use the new Valkey read-write and read-only endpoints which are exposed as services (`SERVICE.NAMESPACE.svc.cluster.local`). In the example above: | ||
|
|
||
| * Read-Write (primary): `valkey.apps-test.svc.cluster.local` | ||
| * Read-Only (all instances): `valkey-read.apps-test.svc.cluster.local` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we tag the endpoints to environment variables?.
|
|
||
| ```shell | ||
| $ kubectl get pods --all-namespaces -l app.kubernetes.io/name=valkey -o custom-columns=Pod:.metadata.name,Namespace:.metadata.namespace,Instance:.metadata.labels.app\\.kubernetes\\.io\\/instance | ||
| Pod Namespace Instance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is 53 to 57 sample outcome from the command?. can we call it out explicitly with a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated all shells where output is shown with # * Sample Output *, not sure it is clearer or not. By default if a shell command is listed with a $ in front, it means to show the shell command and output.
| $ $NEW_VALKEY_CLI replicaof $SVCPRIMARY 6379 | ||
| OK | ||
| $ $NEW_VALKEY_CLI info | grep '^\(role\|master_host\|master_link_status\)' | ||
| role:slave |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what does this statement do?.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding comments might help.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
stockholmux
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix the author bio then I can review :)
Signed-off-by: Silvio Gissi <[email protected]>
stockholmux
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall good.
A few little issues with shell vs bash, using $, and not commenting example output. In general, this is so if someone copy/pastes via double click everything will actually run in a shell instead of trying to execute the sample output.
|
|
||
| ### Step 1: Find existing pods, services and namespace | ||
|
|
||
| ```shell |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shell as a tag block causes Zola to spam the following warning: Warning: Highlight language shell not found in blog/2025-11-05-valkey-helm-chart/index.md - it really should be bash (and you'll get syntax highlighting too)
|
|
||
| ```shell | ||
| # List all pods in all namespaces with app name 'valkey' | ||
| $ kubectl get pods --all-namespaces -l app.kubernetes.io/name=valkey -o custom-columns=Pod:.metadata.name,Namespace:.metadata.namespace,Instance:.metadata.labels.app\\.kubernetes\\.io\\/instance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
drop the $
| # List all pods in all namespaces with app name 'valkey' | ||
| $ kubectl get pods --all-namespaces -l app.kubernetes.io/name=valkey -o custom-columns=Pod:.metadata.name,Namespace:.metadata.namespace,Instance:.metadata.labels.app\\.kubernetes\\.io\\/instance | ||
| # * Sample Output * | ||
| Pod Namespace Instance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment out all example output (or put it in a different code block).
|
|
||
| Install the new Valkey instance: | ||
|
|
||
| ```shell |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shell->bash
|
|
||
| Check it is running as expected: | ||
|
|
||
| ```shell |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bash
|
|
||
| ```shell | ||
| # List new pods and ensure they are in 'Running' state | ||
| $ kubectl get pods -n $NAMESPACE -l app.kubernetes.io/instance=$NEWINSTANCE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no $
| # List new pods and ensure they are in 'Running' state | ||
| $ kubectl get pods -n $NAMESPACE -l app.kubernetes.io/instance=$NEWINSTANCE | ||
| # * Sample Output * | ||
| NAME READY STATUS RESTARTS AGE |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
example output should be commented
Description
This PR supersedes #408, to add a new blog post about the new official Valkey Helm Chart, including reasons for the chart creation and migration steps from the existing chart.
Issues Resolved
Closes #408
Check List
--signoffBy submitting this pull request, I confirm that my contribution is made under the terms of the BSD-3-Clause License.